Improve GeoJSON Parsing #73
Conversation
Sometimes failing due to issues with voices.
The previous strategy used for parsing the OSM GeoJson data received from our services was based on old Objective-C compatibility, which is generally no longer needed. Replacing it provides significant simplification and type safety. First, we now use `JSONDecoder` and the `Decodable` protocol to parse GeoJson data, rather than the old (NS)JSONSerialization strategy. Second, `GeometryType` and `GeoJsonGeometry` have finally been merged into a single enum (which was previously not done due to Objective-C support) - This guarantees that invalid coordinate structure for a geometry type is unrepresentable - We also use CLLocationCoordinate2D instead of keeping Double arrays Since GDASpatialDataResultEntity objects (among others) are cached in the local Realm database, the schema is left unchanged, despite otherwise improving the usage - Eventually, a migration from the legacy Obj-C Realm mode to the new Swift mode should be done, which will also allow objects to be stored more naturally. I would also like to note that the various implementations of GeoJson objects do not entirely adhere to the spec, and it may be preferable to parse GeoJson objects while adhering to spec, and only then convert it into the desired forms.
This improves consistency with naming of GeoJsonGeometry and GeoJsonFeature, as well as reducing clutter.
There was a problem hiding this comment.
I'm not particularly concerned that our GeoJSON implementation is 100% spec-compliant, because we control both the client and the server here so can accommodate exceptions if needed. (Admittedly, the server is deferring to PostGIS to generate that GeoJSON, so it might not be so easy for the server to generate non-standard GeoJSON, either..)
In a perfect world, we would be translating all this feature geometry handling into a platform-independent library that we could use in that oft-requested Android edition. But, Objective-C to Swift is a happy accomplishment, too. Thanks for your work!
apps/ios/GuideDogs/Code/Data/Models/JSON Parsing/OSM/GeoJsonFeature.swift
Outdated
Show resolved
Hide resolved
apps/ios/GuideDogs/Code/Data/Models/JSON Parsing/OSM/GeoJsonFeature.swift
Show resolved
Hide resolved
Despite spec allowing non-located features, we need a location, so will reject features that lack a geometry
|
I think this should be ready to merge, unless somebody else wants to review |
|
@2kai2kai2 Thanks for all the work you put into this! I am having an issue with the app since these changes, It basically doesn't work at all unfortunately. It announces the heading, then says nearest road is over 8000 miles away, but it is a road that is quite near, less than a mile. |
|
@RDMurray I take it you had to revert this merge to get recreation activities working (#84). Can you provide a sample offending activity file? Maybe @2kai2kai2 could add a test case with some real output from the authoring tool. |
|
Note that I only reverted it in my fork. This is unrelated to the fix for the authoring tool. Since this pr Soundscape doesn't work at all for me.. I haven't had a chance to look into why. If you want to try it out, build 15 on testflight includes this pr and build 16 doesn't. Use menu->settings->trouble shooting->clear map data for testing. |
|
Oh, sorry, I was confusing this PR with #65 that replaced the GPX dependencies. I was wary of accepting that before we had authoring tool output to try, but since you didn't need to revert it I think we're clear there. |
|
I've reverted this in main. It still seems to be broken after fixing the swapped coordinates issue. Current location says the correct road, but around me and ahead of me only report markers. Street preview also fails to start. Note for testing: it is necessary to clear map data, the problem seems to be with loading newly downloaded data. |
|
|
||
| extension GeoJsonGeometry { | ||
| private static func into_coord_pair(_ coord: CLLocationCoordinate2D) -> [CLLocationDegrees] { | ||
| return [coord.latitude, coord.longitude] |
There was a problem hiding this comment.
I think the coordinates are swapped here.
GeoJSON files are what we receive from our map data service, and provide all POI/road/map data.
JSONSerializationwith Swift'sJSONDecoderfor parsing of JSON structuresGeoJsonGeometry,GeoJsonFeature, andGeoJsonFeatureCollectionGeoJsonGeometryclass to simplify and reduce code duplicationNote these changes are to be followed by the rest in draft #72
Also note that (and I currently do not plan to do address this), these changes do not fully bring our implementation in line with the GeoJSON spec, as we still do various business logic and custom parsing of properties as a part of parsing GeoJSON objects. If we ever want a more general implementation for GeoJSON, this would have to be changed.